chore(training): correct f-strings; log param counts via utils - #113

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

• X
• Mastodon
• LinkedIn
• Facebook

src/models/emotion_detection/training_pipeline.py (2)

291-293: Validation cadence likely too sparse on small datasets.

val_frequency = max(500, num_batches // 5) means no mid-epoch validation unless you have ≥500 batches. Consider a floor of 1–5% of the epoch with an upper bound.

-        val_frequency = max(500, num_batches // 5)
+        # Validate roughly 5 times per epoch, with a sensible floor and cap
+        val_frequency = max(1, min(500, max(1, num_batches // 5)))
+        # Optional: make this configurable

---

299-365: Gate heavy first-batch diagnostics behind DEBUG level.

These logs compute reductions on GPU tensors and run every epoch’s first batch. They’re excellent during debugging but add overhead in production. Recommend logging them at DEBUG and flipping the logger level based on a debug_mode flag.

I can wire the debug_mode argument (present in train_emotion_detection_model) through to the trainer and switch these logger.info calls to logger.debug while setting logger.setLevel(logging.DEBUG) if enabled.

Also applies to: 368-390, 400-416

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/models/emotion_detection/training_pipeline.py (1)

34-34: Good switch to shared param-count utility.

Importing and using count_model_params(..., only_trainable=True) centralizes logic and prevents drift vs. per-model implementations.

Also applies to: 245-246

src/models/emotion_detection/training_pipeline.py (1)

34-34: Import path consistency and existence of src.utils

This absolute import deviates from the surrounding relative imports and may fail depending on packaging/runtime (src-layout vs installed package). Confirm the module exists and consider aligning import style for consistency.

Run to verify location and signature:

#!/bin/bash
set -euo pipefail

echo "Searching for count_model_params definition:"
rg -nP 'def\s+count_model_params\s*\(' src || true

echo "Listing src/utils locations:"
fd -t f utils src | sed -n '1,200p'

echo "Grepping for count_model_params usage across repo:"
rg -nF 'count_model_params(' || true

src/models/emotion_detection/training_pipeline.py (5)

102-102: PR summary vs implementation: f-strings vs parameterized logging

The PR description says “convert logger calls to f-strings”, but the code uses parameterized logging (logger.info("...", args)), which is generally preferred. Either update the PR description to reflect parameterized logging or switch calls to f-strings to match the stated objective. Don’t mix styles.

---

213-222: Downgrade “DEBUG” diagnostics to debug level and/or gate behind a flag

These are verbose diagnostics but logged at INFO. Consider using logger.debug and a guard (e.g., if logger.isEnabledFor(logging.DEBUG):) to reduce noise in normal runs.

Apply this minimal pattern:

-        logger.info("🔍 DEBUG: Loss Function Analysis")
-        logger.info("   Loss function type: %s", type(self.loss_fn).__name__)
+        if logger.isEnabledFor(logging.DEBUG):
+            logger.debug("Loss Function Analysis")
+            logger.debug("   Loss function type: %s", type(self.loss_fn).__name__)

If you want a flag, introduce self.debug_mode in init and gate on it as well.

---

303-367: Verbose training diagnostics at INFO; consider demoting to DEBUG and gating

Large blocks of per-batch shape/stats logs at INFO will flood logs. Recommend logger.debug with an isEnabledFor(DEBUG) guard and/or a debug_mode flag to explicitly enable them.

Example pattern:

-            if batch_idx == 0:
-                logger.info("🔍 DEBUG: Data Distribution Analysis")
-                logger.info("   Labels shape: %s", labels.shape)
+            if batch_idx == 0 and logger.isEnabledFor(logging.DEBUG):
+                logger.debug("Data Distribution Analysis")
+                logger.debug("   Labels shape: %s", labels.shape)

Repeat for “Model Output Analysis”, “Loss Analysis”, and “Gradient Analysis” sections.

---

618-628: Docstring still advertises debug_mode and new default behavior

Update docstring to remove debug_mode and reflect dev_mode now being opt-in.

Apply:

-        device: Device to use for training (auto-detect if None)
-        dev_mode: Enable development mode with smaller dataset
-        debug_mode: Enable debugging mode with enhanced logging
+        device: Device to use for training (auto-detect if None)
+        dev_mode: If True, use a small subset of data for quicker iterations

---

649-651: Consider exposing dev_mode in CLI/main or environment instead of defaulting in API

train_emotion_detection_model is a convenient API; let callers choose dev_mode explicitly. For CLI, pass via flag or env to avoid accidental partial-data training.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/models/emotion_detection/training_pipeline.py (3)

392-393: Nice fix: robust formatting of clip_grad_norm_ return type

Casting to float avoids TypeError when clip_grad_norm_ returns a Tensor. Good defensive logging.

---

632-637: Logging copy reflects new defaults; adjust messages after reverting dev_mode default

If dev_mode becomes opt-in again, these messages are still fine. If you keep dev_mode default True, this section will fire by default and materially change behavior. Align with the chosen default.

---

246-251: Param count logging: verify utility existence and formatting approach

Please confirm that the count_model_params helper in src/utils is defined, accepts only_trainable=True, and returns an integer as expected. Additionally, since logger.info supports lazy interpolation, you may choose to pass the raw integer and let the logger handle formatting, or pre-format for readability.

• Confirm that count_model_params is implemented in src/utils and its signature includes only_trainable
• Ensure it returns an integer when only_trainable=True
• (Optional) Instead of format(..., ",d"), consider:

• Passing the raw count to logger.info("…%d…", count) for lazy formatting
• Precomputing a formatted string once, if that improves clarity

src/models/emotion_detection/training_pipeline.py (1)

34-34: Import path may be brittle; align with local style and verify utility exists

Absolute from src.utils ... can break outside a src-layout and is inconsistent with nearby relative imports. Prefer a relative import (adjust the level to match your layout) or ensure src is on PYTHONPATH. Also verify the utility actually exists to avoid runtime ImportError.

Apply one of the following (adjust dots as needed):

-from src.utils import count_model_params
+from ..utils import count_model_params  # adjust relative level to your package layout

To verify presence and path quickly:

#!/bin/bash
# Verify utility exists and import path works
set -euo pipefail
echo "Definitions of count_model_params:"
rg -nP '\bdef\s+count_model_params\s*\(' -C1 src || true
python - <<'PY'
try:
    from src.utils import count_model_params
    print("OK: src.utils.count_model_params import works")
except Exception as e:
    print("WARN: src.utils import failed:", e)
PY

src/models/emotion_detection/training_pipeline.py (3)

296-297: Validation frequency can skip validation on small datasets

With val_frequency = max(500, num_batches // 5), small num_batches yields 500 and no mid-epoch validation. Consider clamping to at least 1 and at most 500.

-        val_frequency = max(500, num_batches // 5)
+        # Validate roughly 5x/epoch, but never less than once and never too chatty
+        val_frequency = max(1, min(500, max(1, num_batches // 5)))

---

374-385: Avoid .data in gradient norm calculation

Use .detach() (or directly p.grad.norm(2)) to avoid autograd footguns; .data is discouraged.

-                        param_norm = p.grad.data.norm(2)
+                        param_norm = p.grad.detach().norm(2)

---

639-645: Use debug_mode to dial logging verbosity

Right now, INFO-level diagnostics are very chatty even when debug_mode=False. Consider setting the logger level from this flag.

-    if dev_mode:
+    # Control verbosity early
+    logger.setLevel(logging.INFO if debug_mode else logging.WARNING)
+    if dev_mode:
         logger.info("🚀 DEVELOPMENT MODE ENABLED: Fast training with reduced dataset")
         logger.info("🚀 Expected training time: 30-60 minutes instead of 9 hours")
     else:
         logger.info("🏭 PRODUCTION MODE: Full dataset training")

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/models/emotion_detection/training_pipeline.py (11)

102-102: Good switch to parameterized logging for device message

Lazy formatting via logger args avoids unnecessary string interpolation. LGTM.

---

161-166: Dev-mode batch-size bump is reasonable and well logged

Clear, bounded increase with informative logging. LGTM.

---

194-196: Dataset size logging converted correctly

Interpolations are now reliable; no stray braces. LGTM.

---

214-223: Loss/class-weight diagnostics look solid

Casts to scalars with .item() avoid Tensor-format errors. LGTM.

---

247-252: Param-count logging via utility: OK, minor nit

Using format(..., ",d") yields a readable, grouped number and is safe with %s. No action needed.

---

319-325: Per-class positives logging is correct and efficient

Limited to first 10 classes and scalarized counts. LGTM.

---

395-400: Nice robustness on clip_grad_norm_ return type

Casting to float avoids formatting errors when a Tensor is returned. LGTM.

---

445-448: Epoch summary logging fixed

Interpolations are now reliable and readable. LGTM.

---

461-482: Validation and patience logging improvements look good

Clear epoch context and patience tracking with safe formatting. LGTM.

---

516-520: Checkpoint naming and save log are fine

Explicit file name and path logging is clear. LGTM.

---

521-545: Order of operations in train() is sound

Data prep → initialize → train/validate loop with early stopping. LGTM.

src/models/emotion_detection/training_pipeline.py (2)

34-34: Import style + module availability: prefer relative import or provide fallback; verify count_model_params exists

This absolute import breaks consistency with nearby relative imports and may fail outside a src-layout. Provide a fallback relative import, or standardize on one style. Also verify the utility actually exists in the repo/package.

Apply this safer import pattern:

-from src.utils import count_model_params
+try:
+    # src-layout (editable install) support
+    from src.utils import count_model_params
+except ModuleNotFoundError:
+    # package-relative fallback for installed distributions
+    from ..utils import count_model_params

Run to verify presence/signature (no imports executed):

#!/bin/bash
rg -nP 'def\s+count_model_params\s*\(' -n src || { echo "Missing count_model_params utility"; exit 1; }
rg -nP '\bcount_model_params\(' -n src/models/emotion_detection/training_pipeline.py

---

373-377: Fix: boolean comparison on tensors is ambiguous

Using labels.sum() == 0 yields a Tensor; if on a Tensor raises “boolean value of Tensor is ambiguous”.

-        if labels.sum() == 0:
+        total_positives = int(labels.sum().item())
+        if total_positives == 0:
             logger.error("❌ CRITICAL: All labels are zero!")
-        elif labels.sum() == labels.numel():
+        elif total_positives == labels.numel():
             logger.error("❌ CRITICAL: All labels are one!")

src/models/emotion_detection/training_pipeline.py (6)

241-247: Clamp warmup steps to avoid scheduler misconfiguration when total_steps is small

If total_steps < warmup_steps the scheduler can be ill‑configured (and occasionally error depending on HF version). Clamp and log the adjustment.

-        total_steps = len(self.train_dataloader) * self.num_epochs
+        total_steps = len(self.train_dataloader) * self.num_epochs
+        # Guard: avoid warmup > total steps
+        effective_warmup = min(self.warmup_steps, max(0, total_steps - 1))
...
-        self.scheduler = get_linear_schedule_with_warmup(
+        self.scheduler = get_linear_schedule_with_warmup(
             self.optimizer,
-            num_warmup_steps=self.warmup_steps,
+            num_warmup_steps=effective_warmup,
             num_training_steps=total_steps,
         )
+        if effective_warmup != self.warmup_steps:
+            logger.info(
+                "Adjusted warmup steps from %d to %d to fit total steps=%d",
+                self.warmup_steps, effective_warmup, total_steps
+            )

---

249-254: Param-count logging works; minor nit on pre-formatting

Works as-is. If you want to keep lazy logging benefits, pre-format once and pass as an arg rather than formatting within the call.

-        logger.info(
-            "Model initialized with %s trainable parameters",
-            format(count_model_params(self.model, only_trainable=True), ",d"),
-        )
+        param_count = count_model_params(self.model, only_trainable=True)
+        logger.info("Model initialized with %s trainable parameters", f"{param_count:,}")

---

421-433: Avoid .data; use .detach() for grad norm computation

Accessing .data is discouraged and can lead to subtle autograd issues. Use .grad.detach().

-        for p in self.model.parameters():
-            if p.grad is not None:
-                param_norm = p.grad.data.norm(2)
+        for p in self.model.parameters():
+            if p.grad is not None:
+                param_norm = p.grad.detach().norm(2)
                 total_norm += param_norm.item() ** 2
                 param_count += 1

---

439-442: Casting clip norm: prefer .item() for tensors

float(tensor) typically works for 0-D tensors, but .item() is explicit and avoids surprises across devices.

-        clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm
+        clip_val = clip_norm.item() if isinstance(clip_norm, torch.Tensor) else clip_norm
         logger.info("   Gradient norm after clipping: %.6f", clip_val)

---

560-571: Skip duplicate validation when early stop triggers mid-epoch

When _maybe_validate_and_early_stop returns early, train() still performs per-epoch validation immediately after, duplicating work. Short-circuit if early_stopped is present.

         for epoch in range(1, self.num_epochs + 1):
             train_metrics = self.train_epoch(epoch)

             if self.evaluation_strategy == "epoch":
+                if train_metrics.get("early_stopped"):
+                    self.training_history.append(train_metrics)
+                    logger.info("Early stop: skipping end-of-epoch validation at epoch %d", epoch)
+                    break
                 val_metrics = self.validate(epoch)

---

102-102: Logging style is consistent but diverges from the PR objective

You’ve standardized on parameterized logging ("%s"/"%.4f") rather than f-strings. That’s fine (and preferred for lazy formatting), but it conflicts with the PR description claim “convert to f-strings”. Consider updating the PR text to avoid confusion.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/models/emotion_detection/training_pipeline.py (4)

646-647: Breaking API/behavior: default dev_mode switched to False; PR text claims “behavior unchanged”

Changing dev_mode default from True→False (and logging production-mode by default) alters runtime behavior and expected training time. Either revert the default, or update PR description, docs, and any callers.

• Confirm previous default and intended behavior.
• If keeping the new default, add a short note in the docstring and README/CHANGELOG.

Also applies to: 664-668

---

214-225: Loss/class-weight debug logging looks solid

Nice use of logger.isEnabledFor(logging.DEBUG) gating and .item() extraction for scalars.

---

354-357: Epoch summary logging: solid and concise

Clear, numeric formatting and consistent units.

---

542-546: Checkpoint naming/logging LGTM

The f-string path and post-save info log are clear and consistent.

src/models/emotion_detection/training_pipeline.py (2)

22-22: Import style consistency: consider relative import or keep consistent with rest of file.

This absolute import differs from the relative imports used below. Stick to one style for maintainability. If src.utils is a top-level util by design, ignore this.

Would you prefer me to switch to a relative import (e.g., from ...utils import count_model_params) if utils lives under src, or keep absolute across the file?

---

378-406: Fix ambiguous Tensor booleans in label checks.

Using labels.sum() directly in if statements raises “boolean value of Tensor is ambiguous”.

Apply:

-        if labels.sum() == 0:
+        if labels.sum().item() == 0:
             logger.error("❌ CRITICAL: All labels are zero!")
-        elif labels.sum() == labels.numel():
+        elif labels.sum().item() == labels.numel():
             logger.error("❌ CRITICAL: All labels are one!")

src/models/emotion_detection/hf_loader.py (4)

186-194: Replace blanket try/except-pass with debug logging to aid diagnostics.

Several branches swallow exceptions silently. At minimum, log at debug with the source that failed so operators can troubleshoot without enabling full tracing.

Proposed minimal changes:

-        except Exception:
-            pass
+        except Exception as e:
+            logger.debug("Local dir load failed for %s: %s", local_dir, e)

Repeat similarly for the HF Hub direct/snapshot and archive branches. Add import logging and logger = logging.getLogger(__name__) at module top if not present.

Also applies to: 214-229

---

176-183: Pathlib and platform-appropriate cache dirs.

• Prefer pathlib.Path over os.path.* for readability and to satisfy PTH1xx hints.
• Defaults like "/var/tmp/hf-cache" can trip security auditors. Consider using Hugging Face defaults or platform caches (e.g., appdirs.user_cache_dir).

Example:

-from os.path import join, dirname, basename, exists, isdir
-from pathlib import Path
+from pathlib import Path

-cache_base = os.getenv("HF_HOME", "/var/tmp/hf-cache")
-snap_dir = snapshot_download(repo_id=model_id, token=token, cache_dir=cache_base)
+cache_base = Path(os.getenv("HF_HOME", str(Path.home() / ".cache" / "huggingface"))))
+snap_dir = snapshot_download(repo_id=model_id, token=token, cache_dir=str(cache_base))

Also applies to: 189-194, 215-217, 218-221

---

66-77: threshold argument is unused in remote inference.

The predict signature exposes threshold, but the value isn’t applied. Either:

• Use it to filter or choose labels (align with HFEmotionDetector), or
• Drop it from the signature for consistency.

Would you like me to wire threshold-based top-K/threshold filtering to match local model behavior?

Also applies to: 84-99

---

214-221: Candidate discovery could be more selective.

Scanning extract_dir plus all first-level subdirs is pragmatic, but consider limiting to directories containing both config.json and either pytorch_model.bin or model.safetensors to cut false positives.

I can propose a small helper that validates a candidate dir before attempting _wrap_local_model.

src/security/jwt_manager.py (3)

49-51: Comment is stale: Token pair is not a plain dict.

The code returns a TokenResponse pydantic model, not a plain dict. Update the comment to avoid confusion.

-# Token pair is returned as a plain dict
+# Token pair is returned as a TokenResponse model

---

69-71: Use timezone-aware UTC and consistent numeric timestamps.

Naive datetimes (utcnow/fromtimestamp) trigger linter warnings and can cause subtle bugs. Prefer timezone-aware UTC and store numeric seconds in JWTs for consistency.

-from datetime import datetime, timedelta
+from datetime import datetime, timedelta, timezone
...
-            "exp": datetime.utcnow() + timedelta(minutes=ACCESS_TOKEN_EXPIRE_MINUTES),
-            "iat": datetime.utcnow(),
+            "exp": datetime.now(timezone.utc) + timedelta(minutes=ACCESS_TOKEN_EXPIRE_MINUTES),
+            "iat": datetime.now(timezone.utc),
...
-            "exp": datetime.utcnow() + timedelta(days=REFRESH_TOKEN_EXPIRE_DAYS),
-            "iat": datetime.utcnow(),
+            "exp": datetime.now(timezone.utc) + timedelta(days=REFRESH_TOKEN_EXPIRE_DAYS),
+            "iat": datetime.now(timezone.utc),
...
-            exp_datetime = (
-                datetime.fromtimestamp(exp_timestamp) if exp_timestamp else None
-            )
+            exp_datetime = (
+                datetime.fromtimestamp(exp_timestamp, tz=timezone.utc) if exp_timestamp else None
+            )

Alternatively, encode exp/iat as ints via .timestamp() to avoid tz conversion later.

Also applies to: 81-85, 133-136

---

105-113: Avoid logging token material; prefer jti or a hash.

Even partial token logs can be sensitive. Consider logging a hash or a JWT ID (jti) claim instead.

-            logger.warning(f"Token expired: {token[:10]}...")
+            logger.warning("Token expired: %s", hashlib.sha256(token.encode()).hexdigest()[:12])

Or add a jti claim at creation and log that.

src/security_headers.py (5)

2-5: Docstring nits: Punctuation and spacing.

Minor style fixes per D205/D415: end the summary with punctuation and leave a blank line before the description.

-"""🛡️ Security Headers Middleware
-==============================
-Flask middleware for adding security headers and implementing security policies.
-"""
+"""🛡️ Security Headers Middleware.
+
+==============================
+Flask middleware for adding security headers and implementing security policies.
+"""

---

68-75: Prefer pathlib and configurable config path.

Pathlib improves readability, and a configurable security.yaml path (e.g., via Flask config or env) aids deployment across environments.

-            config_path = os.path.join(
-                os.path.dirname(__file__), "../configs/security.yaml"
-            )
-            with open(config_path) as f:
+            from pathlib import Path
+            base_dir = Path(__file__).resolve().parent
+            config_path = Path(os.getenv("SECURITY_CONFIG", base_dir / "../configs/security.yaml")).resolve()
+            with config_path.open() as f:
                 security_config = yaml.safe_load(f)

---

92-99: PII/log volume caution for security logs.

Logging remote_addr, X-Forwarded-For, and raw UA may be considered PII. Consider:

• Masking IPs (e.g., /24 aggregation) or hashing.
• Sampling logs in high-traffic paths.
• Making individual fields opt-in via SecurityHeadersConfig.

I can add config flags like redact_ips/redact_user_agent and a small helper to redact before logging—want me to draft it?

Also applies to: 220-236, 463-483

---

246-317: Deduplicate and precompile UA pattern lists.

• "checker" appears twice in low_risk_patterns.
• Consider converting lists to tuples or sets and deduplicating to avoid double counting.
• You can also precompute lowercase sets once for a small perf win.

-        low_risk_patterns = [
+        low_risk_patterns = [
             "indexer",
             "feed",
             "rss",
             "aggregator",
             "monitor",
-            "checker",
+            "checker",
             "validator",
             "linter",
-            "checker",
             "analyzer",
         ]

If you’d like, I can send a follow-up diff to use sets and avoid repeated scoring.

Also applies to: 318-331

---

24-44: Config surface duplication is confusing (enable_csp vs enable_content_security_policy, enable_hsts vs enable_strict_transport_security).

You expose both names but only use the latter ones. Either:

• Drop the unused duplicates, or
• Wire them as synonyms to reduce confusion.

Happy to prep a cleanup commit mapping legacy names to the new ones to avoid breaking external code.

Also applies to: 487-516

src/models/emotion_detection/training_pipeline.py (1)

479-491: Minor: simplify clip_norm cast.

Use a one-liner conditional cast (SIM108). No behavioral change.

-        if not isinstance(clip_norm, (int, float)):
-            clip_val = float(clip_norm)
-        else:
-            clip_val = clip_norm
+        clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/models/emotion_detection/training_pipeline.py (4)

645-653: Fix metric keys: use f1_macro from evaluator.

evaluate_emotion_classifier returns f1_macro/f1_micro. Accessing macro_f1 will raise KeyError and break early-stopping/best-model logic.

-        current_score = val_metrics["macro_f1"]
+        current_score = val_metrics["f1_macro"]
@@
-                logger.info("New best model saved! Macro F1: %.4f", current_score)
+                logger.info("New best model saved! Macro F1: %.4f", current_score)

---

314-323: Fix NameError and misnamed arg: pass val_frequency and start_time + rename avg_loss→total_loss.

_maybe_validate_and_early_stop references val_frequency and start_time that are not in scope, and you’re passing total_loss into a parameter named avg_loss. This will throw at runtime and also miscomputes train loss.

Apply at call site:

-            maybe_metrics = self._maybe_validate_and_early_stop(
-                batch_idx, epoch, num_batches, total_loss, self.scheduler.get_last_lr()[0]
-            )
+            maybe_metrics = self._maybe_validate_and_early_stop(
+                batch_idx=batch_idx,
+                epoch=epoch,
+                num_batches=num_batches,
+                total_loss=total_loss,
+                current_lr=self.scheduler.get_last_lr()[0],
+                val_frequency=val_frequency,
+                start_time=start_time,
+            )

---

593-627: Finalize _maybe_validate_and_early_stop API to match call site and fix docstring.

Accept val_frequency and start_time, and use total_loss to compute average correctly. Also fix the Returns docstring (it returns Optional[Dict], not bool).

-    def _maybe_validate_and_early_stop(
-        self,
-        batch_idx: int,
-        epoch: int,
-        num_batches: int,
-        avg_loss: float,
-        current_lr: float,
-    ) -> Optional[Dict[str, Any]]:
+    def _maybe_validate_and_early_stop(
+        self,
+        batch_idx: int,
+        epoch: int,
+        num_batches: int,
+        total_loss: float,
+        current_lr: float,
+        val_frequency: int,
+        start_time: float,
+    ) -> Optional[Dict[str, Any]]:
@@
-        Returns:
-            bool: True if training should stop early, False otherwise
+        Returns:
+            Optional[Dict[str, Any]]: Metrics snapshot if early-stopped, else None
@@
-        if (batch_idx + 1) % val_frequency != 0:
+        if (batch_idx + 1) % val_frequency != 0:
             return None
@@
-            return {
+            return {
                 "epoch": epoch,
-                "train_loss": avg_loss / (batch_idx + 1),
-                "epoch_time": time.time() - start_time,
+                "train_loss": total_loss / (batch_idx + 1),
+                "epoch_time": time.time() - start_time,
                 "learning_rate": current_lr,
                 "early_stopped": True,
             }

---

456-470: Fix ambiguous Tensor boolean in label sanity checks.

if labels.sum() == 0: and == labels.numel() yield tensor booleans; branching on them raises “boolean value of Tensor is ambiguous”.

-        if labels.sum() == 0:
+        pos_sum = int(labels.sum().item())
+        if pos_sum == 0:
             logger.error("❌ CRITICAL: All labels are zero!")
-        elif labels.sum() == labels.numel():
+        elif pos_sum == labels.numel():
             logger.error("❌ CRITICAL: All labels are one!")

src/models/emotion_detection/training_pipeline.py (3)

253-260: Guard against warmup > total_steps.

If len(train_dataloader) * num_epochs ≤ warmup_steps, the scheduler will spend the entire training warming up. Clamp warmup to total_steps - 1.

-        self.scheduler = get_linear_schedule_with_warmup(
-            self.optimizer,
-            num_warmup_steps=self.warmup_steps,
-            num_training_steps=total_steps,
-        )
+        self.scheduler = get_linear_schedule_with_warmup(
+            self.optimizer,
+            num_warmup_steps=min(self.warmup_steps, max(0, total_steps - 1)),
+            num_training_steps=total_steps,
+        )

---

557-561: One-liner for casting clip_norm (SIM108).

Minor style: use a ternary.

-        if not isinstance(clip_norm, (int, float)):
-            clip_val = float(clip_norm)
-        else:
-            clip_val = clip_norm
+        clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm

---

219-235: Debug gating is good; consider logger.debug within debug-only helpers.

You guard calls with logger.isEnabledFor(DEBUG) but inner logs use info. Switching to debug keeps log levels semantically aligned; no functional change.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/models/emotion_detection/training_pipeline.py (4)

22-25: Import style consistency: prefer one style (relative vs absolute) within this module

This file uses relative imports for local modules (Lines 24–25) but an absolute import for src.utils (Line 22). Pick one style for maintainability; relative would be from ...utils import count_model_params given the package layout.

-from src.utils import count_model_params
+from ...utils import count_model_params

---

314-317: Undefined names downstream: pass val_frequency and start_time; also pass the correct loss accumulator

_maybe_validate_and_early_stop references val_frequency and start_time but they’re not in scope inside that method. Also, you pass total_loss but name the parameter avg_loss inside the callee. Pass both missing values and align the parameter name.

-            maybe_metrics = self._maybe_validate_and_early_stop(
-                batch_idx, epoch, num_batches, total_loss, self.scheduler.get_last_lr()[0]
-            )
+            maybe_metrics = self._maybe_validate_and_early_stop(
+                batch_idx=batch_idx,
+                epoch=epoch,
+                num_batches=num_batches,
+                total_loss=total_loss,
+                current_lr=self.scheduler.get_last_lr()[0],
+                val_frequency=val_frequency,
+                start_time=start_time,
+            )

---

463-467: Fix ambiguous Tensor boolean comparisons

labels.sum() == 0 and labels.sum() == labels.numel() yield Tensor booleans, which are invalid in if. Convert to Python scalars.

-        if labels.sum() == 0:
+        total_positives = int(labels.sum().item())
+        if total_positives == 0:
             logger.error("❌ CRITICAL: All labels are zero!")
-        elif labels.sum() == labels.numel():
+        elif total_positives == labels.numel():
             logger.error("❌ CRITICAL: All labels are one!")

---

585-618: Fix _maybe_validate_and_early_stop: undefined names, wrong param naming, and incorrect return docstring

• Uses val_frequency and start_time without defining them.
• Parameter avg_loss is actually the running total_loss.
• Docstring “Returns: bool …” contradicts actual return type (Optional[Dict[str, Any]]).

Refactor signature and internal usage accordingly.

-    def _maybe_validate_and_early_stop(
-        self,
-        batch_idx: int,
-        epoch: int,
-        num_batches: int,
-        avg_loss: float,
-        current_lr: float,
-    ) -> Optional[Dict[str, Any]]:
-        """Check if validation should be performed and handle early stopping.
+    def _maybe_validate_and_early_stop(
+        self,
+        batch_idx: int,
+        epoch: int,
+        num_batches: int,
+        total_loss: float,
+        current_lr: float,
+        val_frequency: int,
+        start_time: float,
+    ) -> Optional[Dict[str, Any]]:
+        """Check if validation should be performed and handle early stopping.
@@
-        Returns:
-            bool: True if training should stop early, False otherwise
+        Returns:
+            Optional[Dict[str, Any]]: Metrics snapshot if early-stopped; otherwise None.
@@
-        if (batch_idx + 1) % val_frequency != 0:
+        if (batch_idx + 1) % val_frequency != 0:
             return None
         logger.info(f"🔍 Validating at batch {batch_idx + 1}...")
         self.validate(epoch)
         if self.should_stop_early():
             logger.info(f"🛑 Early stopping triggered at batch {batch_idx + 1}")
             return {
                 "epoch": epoch,
-                "train_loss": avg_loss / (batch_idx + 1),
+                "train_loss": total_loss / (batch_idx + 1),
                 "epoch_time": time.time() - start_time,
                 "learning_rate": current_lr,
                 "early_stopped": True,
             }
         return None

src/utils.py (1)

5-5: Remove unused import to satisfy Ruff F401

from typing import Union is unused in this module. Drop it to keep the file clean and pass linting.

-from typing import Union

src/models/emotion_detection/training_pipeline.py (5)

371-379: Log “before clipping” truly before clipping

You compute clip_grad_norm_ (in‑place) and then log “before” stats. Swap order so diagnostics are accurate.

-        # Gradient clipping
-        clip_norm = torch.nn.utils.clip_grad_norm_(
-            self.model.parameters(), max_norm=1.0
-        )
-
-        # Log gradient stats for first batch
-        if batch_idx == 0:
-            self._log_gradient_stats_before()
-            self._log_gradient_stats_after(clip_norm)
+        # Log gradient stats for first batch (before clipping)
+        if batch_idx == 0:
+            self._log_gradient_stats_before()
+
+        # Gradient clipping
+        clip_norm = torch.nn.utils.clip_grad_norm_(self.model.parameters(), max_norm=1.0)
+
+        # Log gradient stats for first batch (after clipping)
+        if batch_idx == 0:
+            self._log_gradient_stats_after(clip_norm)

---

535-541: Avoid .data usage on grads; use .detach() or tensor directly

Accessing p.grad.data is discouraged. Use p.grad (it’s already a Tensor) or p.grad.detach() when computing norms.

-            if p.grad is not None:
-                param_norm = p.grad.data.norm(2)
+            if p.grad is not None:
+                param_norm = p.grad.detach().norm(2)
                 total_norm += param_norm.item() ** 2

---

554-559: Micro cleanup: simplify clip_val assignment

Use a conditional expression as suggested by Ruff SIM108.

-        if not isinstance(clip_norm, (int, float)):
-            clip_val = float(clip_norm)
-        else:
-            clip_val = clip_norm
+        clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm

---

446-462: Level mismatch: debug-gated blocks log at INFO

These helpers early-return unless DEBUG is enabled, yet use logger.info for their messages. Consider switching to logger.debug to match intent (applies in _log_data_distribution, _log_model_output, _log_loss_analysis).

Example:

-        logger.info("🔍 DEBUG: Data Distribution Analysis")
+        logger.debug("🔍 DEBUG: Data Distribution Analysis")

---

384-391: Unify periodic progress logging via _log_progress

You already have _log_progress, but here you inline a different format. Consider delegating to _log_progress for consistency and easier tuning.

-            logger.info(
-                "Epoch %d, Batch %d/%d, Loss: %.8f",
-                epoch, batch_idx + 1, num_batches, loss.item()
-            )
+            self._log_progress(epoch, batch_idx, num_batches, total_loss=total_loss + 0.0)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/utils.py (2)

5-5: Remove unused import to satisfy linter (F401).

typing.Union is imported but not used.

-from typing import Union

---

1-1: Shebang is unnecessary for a library module.

This file isn’t intended to be executed directly; consider removing the shebang to avoid implying executability.

-#!/usr/bin/env python3

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/models/emotion_detection/training_pipeline.py (4)

22-22: Import style: prefer relative to stay consistent with local imports

Other imports in this module use relative paths; switch this one as well for consistency across the file.

-from src.utils import count_model_params
+from ...utils import count_model_params

---

381-390: Log “before clipping” stats before calling clip_grad_norm_

clip_grad_norm_ updates gradients in-place. Currently “before” is logged after clipping, which is misleading. Swap the order.

-        # Gradient clipping
-        clip_norm = torch.nn.utils.clip_grad_norm_(
-            self.model.parameters(), max_norm=1.0
-        )
-
-        # Log gradient stats for first batch
-        if batch_idx == 0:
-            self._log_gradient_stats_before()
-            self._log_gradient_stats_after(clip_norm)
+        # Log gradient stats for first batch (before clipping)
+        if batch_idx == 0:
+            self._log_gradient_stats_before()
+
+        # Gradient clipping
+        clip_norm = torch.nn.utils.clip_grad_norm_(self.model.parameters(), max_norm=1.0)
+
+        # Log gradient stats for first batch (after clipping)
+        if batch_idx == 0:
+            self._log_gradient_stats_after(clip_norm)

---

483-487: Fix ambiguous Tensor boolean checks in logging helper

labels.sum() == 0 yields a Tensor; using it in if raises “boolean value of Tensor is ambiguous”. Convert to Python scalars.

-        if labels.sum() == 0:
+        if labels.sum().item() == 0:
             logger.error("❌ CRITICAL: All labels are zero!")
-        elif labels.sum() == labels.numel():
+        elif labels.sum().item() == labels.numel():
             logger.error("❌ CRITICAL: All labels are one!")

---

315-327: Correct loss variable semantics and make call explicit

You pass total_loss but the callee parameter is named avg_loss and then divided again, producing a double-division error in the returned metrics when early stopping triggers. Rename the parameter to total_loss and compute the average inside; also use keyword args to prevent future ordering bugs.

-            maybe_metrics = self._maybe_validate_and_early_stop(
-                batch_idx,
-                epoch,
-                num_batches,
-                total_loss,
-                self.scheduler.get_last_lr()[0],
-                val_frequency,
-                start_time,
-            )
+            maybe_metrics = self._maybe_validate_and_early_stop(
+                batch_idx=batch_idx,
+                epoch=epoch,
+                num_batches=num_batches,
+                total_loss=total_loss,
+                current_lr=self.scheduler.get_last_lr()[0],
+                val_frequency=val_frequency,
+                start_time=start_time,
+            )
@@
-    def _maybe_validate_and_early_stop(
+    def _maybe_validate_and_early_stop(
         self,
         batch_idx: int,
         epoch: int,
         num_batches: int,
-        avg_loss: float,
+        total_loss: float,
         current_lr: float,
         val_frequency: int,
         start_time: float,
     ) -> Optional[Dict[str, Any]]:
@@
-            avg_loss: Average loss for current epoch
+            total_loss: Accumulated loss so far in the current epoch
@@
-            return {
+            return {
                 "epoch": epoch,
-                "train_loss": avg_loss / (batch_idx + 1),
+                "train_loss": total_loss / (batch_idx + 1),
                 "epoch_time": time.time() - start_time,
                 "learning_rate": current_lr,
                 "early_stopped": True,
             }

Also applies to: 610-648

src/models/emotion_detection/training_pipeline.py (3)

574-578: Nit: simplify cast per Ruff SIM108

Inline the cast using a ternary for brevity; behavior unchanged.

-        if not isinstance(clip_norm, (int, float)):
-            clip_val = float(clip_norm)
-        else:
-            clip_val = clip_norm
+        clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm

---

307-309: Validation frequency guard: avoid “never validate within epoch” on small datasets

max(500, num_batches // 5) means for typical dev runs with <500 batches you will never hit intra-epoch validation. If you intend step-based validation, consider max(1, num_batches // 5). If you only validate per-epoch (default), consider skipping this block entirely when evaluation_strategy == "epoch".

-        val_frequency = max(500, num_batches // 5)
+        val_frequency = max(1, num_batches // 5)

---

473-482: Use debug level inside debug-only helper

This helper runs only when DEBUG is enabled; emitting INFO-level lines can add noise if the gate changes later. Prefer logger.debug here for consistency.

-        logger.info("🔍 DEBUG: Data Distribution Analysis")
-        logger.info("   Labels shape: %s", labels.shape)
-        logger.info("   Labels dtype: %s", labels.dtype)
-        logger.info("   Labels min: %s", labels.min().item())
-        logger.info("   Labels max: %s", labels.max().item())
-        logger.info("   Labels mean: %.6f", labels.float().mean().item())
-        logger.info("   Labels sum: %s", labels.sum().item())
-        logger.info("   Non-zero labels: %s", (labels > 0).sum().item())
-        logger.info("   Total labels: %s", labels.numel())
+        logger.debug("🔍 DEBUG: Data Distribution Analysis")
+        logger.debug("   Labels shape: %s", labels.shape)
+        logger.debug("   Labels dtype: %s", labels.dtype)
+        logger.debug("   Labels min: %s", labels.min().item())
+        logger.debug("   Labels max: %s", labels.max().item())
+        logger.debug("   Labels mean: %.6f", labels.float().mean().item())
+        logger.debug("   Labels sum: %s", labels.sum().item())
+        logger.debug("   Non-zero labels: %s", (labels > 0).sum().item())
+        logger.debug("   Total labels: %s", labels.numel())

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/models/emotion_detection/training_pipeline.py (4)

22-25: Prefer consistent relative import for in-repo modules

To align with the relative imports used elsewhere in this file (e.g., Lines 24–25), import the utility via a relative path. This avoids mixing import styles and eases refactors if the top-level package name changes.

-from src.utils import count_model_params
+from ...utils import count_model_params

If the package layout lacks the necessary init.py files, keep the absolute import; otherwise, prefer the relative one for consistency.

---

381-390: “Before clipping” stats are logged after clipping — swap order

clip_grad_norm_ mutates gradients in-place. Log pre-clip stats before calling it, then log the returned post-clip norm.

-        # Gradient clipping
-        clip_norm = torch.nn.utils.clip_grad_norm_(
-            self.model.parameters(), max_norm=1.0
-        )
-
-        # Log gradient stats for first batch
-        if batch_idx == 0:
-            self._log_gradient_stats_before()
-            self._log_gradient_stats_after(clip_norm)
+        # Log gradient stats for first batch (before clipping)
+        if batch_idx == 0:
+            self._log_gradient_stats_before()
+
+        # Gradient clipping
+        clip_norm = torch.nn.utils.clip_grad_norm_(self.model.parameters(), max_norm=1.0)
+
+        # Log gradient stats for first batch (after clipping)
+        if batch_idx == 0:
+            self._log_gradient_stats_after(clip_norm)

---

483-487: Fix ambiguous tensor booleans in label-distribution checks

labels.sum() == 0 and labels.sum() == labels.numel() produce tensor booleans; use .item() to branch with Python booleans.

-        if labels.sum() == 0:
+        sum_pos = int(labels.sum().item())
+        if sum_pos == 0:
             logger.error("❌ CRITICAL: All labels are zero!")
-        elif labels.sum() == labels.numel():
+        elif sum_pos == labels.numel():
             logger.error("❌ CRITICAL: All labels are one!")

---

804-808: Final-summary metric keys must match evaluator output

Switch to f1_macro and f1_micro to avoid KeyError at runtime.

-        logger.info("Final test Macro F1: %.4f", test_metrics["macro_f1"])
-        logger.info("Final test Micro F1: %.4f", test_metrics["micro_f1"])
+        logger.info("Final test Macro F1: %.4f", test_metrics["f1_macro"])
+        logger.info("Final test Micro F1: %.4f", test_metrics["f1_micro"])

src/models/emotion_detection/training_pipeline.py (3)

545-566: Avoid .data in gradient norm analysis

Using .data bypasses autograd semantics; prefer .detach() for safe, read-only tensors.

-            if p.grad is not None:
-                param_norm = p.grad.data.norm(2)
+            if p.grad is not None:
+                param_norm = p.grad.detach().norm(2)
                 total_norm += param_norm.item() ** 2

---

574-579: Simplify clip-norm casting (Ruff SIM108)

Tighten the cast to a single expression.

-        if not isinstance(clip_norm, (int, float)):
-            clip_val = float(clip_norm)
-        else:
-            clip_val = clip_norm
+        clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm

---

4-6: Docstring overclaims features not present here

The module-level docstring mentions “focal loss, temperature scaling, and ensemble methods,” which aren’t implemented in this file. Consider trimming to avoid confusing readers.

-This module provides a comprehensive training pipeline for the BERT-based emotion
-detection model with advanced features like focal loss, temperature scaling, and
-ensemble methods.
+This module provides a training pipeline for a BERT-based emotion
+detection model with progressive unfreezing, detailed logging, and evaluation utilities.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.